[MOB1-1742] 배너 스펙 확장 - left, right icon을 구분하여 설정할 수 있돌고 수정 #106
Conversation
WalkthroughBanner 공개 API가 Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as 호출자
participant Banner as Banner(...)
participant Layout as BannerLayout
participant UI as Compose UI
Caller->>Banner: 호출 (leftIcon?, leftIconColor?, rightIcon?, description, ...)
Banner->>Layout: 위임 (leftIcon, leftIconColor, rightIcon, description, ...)
alt leftIcon 제공됨
rect #E8F0FE
Note over Layout: 왼쪽 아이콘 렌더링\n(틴트: leftIconColor 또는 기본)
Layout->>UI: Render left icon
end
else
Note over Layout: 왼쪽 아이콘 없음
end
alt rightIcon 제공됨
rect #E8F8EE
Note over Layout: 오른쪽 아이콘 렌더링
Layout->>UI: Render right icon
end
else
rect #FFF7E6
Note over Layout: 기본 오른쪽 chevron 렌더링
Layout->>UI: Render default chevron
end
end
Layout->>UI: Render title / description / content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bezier/src/main/java/io/channel/bezier/component/Banner.kt (3)
114-146: 새 오버로드: rightIcon 노출 조건을 명문화(문서화)하세요현재 구현은 onRemove != null이면 rightIcon이 무시되고, onClick != null일 때만(우측) 아이콘이 표시됩니다. 혼선을 줄이기 위해 KDoc로 명시해 주세요.
@@ -@Composable -fun Banner( +/** + * - leftIcon: 선행 아이콘. + * - rightIcon: onClick != null && onRemove == null 인 경우에만 표시됩니다. + */ +@Composable +fun Banner( @@ -@Composable -fun Banner( +/** + * - leftIcon: 선행 아이콘. + * - rightIcon: onClick != null && onRemove == null 인 경우에만 표시됩니다. + */ +@Composable +fun Banner(또한 제품 요구에 따라 우측 아이콘 색상 커스터마이즈가 필요하면 rightIconColor(Color = Color.Unspecified) 추가를 검토해 주세요.
Also applies to: 147-178
256-269: 접근성: 제거 아이콘에 contentDescription 추가 필요상호작용 요소(onRemove)에 설명이 없어 보조기기에 비노출됩니다. 문자열 리소스를 사용해 설명을 제공하세요.
@@ - Icon( + Icon( modifier = Modifier .size(30.dp) .clickable( interactionSource = remember { MutableInteractionSource() }, indication = null, onClick = onRemove, ) .padding(5.dp), painter = painterResource(id = R.drawable.icon_cancel_small), tint = BezierTheme.colors.txtBlackDark, - contentDescription = null, + contentDescription = stringResource(id = R.string.cd_banner_remove), )추가로 다음 import와 문자열 리소스 정의가 필요합니다.
+import androidx.compose.ui.res.stringResourceres/values/strings.xml:
<string name="cd_banner_remove">배너 닫기</string>
313-315: 미리보기 단순화: 불필요한 leftIconColor 지정 제거기본값이 Color.Unspecified이고 내부에서 테마 색(BezierTheme.colors.txtBlackDark)로 처리되어 현재 지정은 중복입니다. 미리보기 가독성을 위해 제거를 권장합니다.
- leftIconColor = BezierTheme.colors.txtBlackDark,Also applies to: 327-329, 342-343, 365-366, 398-399, 453-454
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bezier/src/main/java/io/channel/bezier/component/Banner.kt(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check-build
🔇 Additional comments (5)
bezier/src/main/java/io/channel/bezier/component/Banner.kt (5)
69-71: 레거시 → 신규 파라미터 브리지 매핑 적절기존 icon/iconColor를 leftIcon/leftIconColor로 정확히 전달하고 rightIcon은 명시적으로 null 처리한 점 좋습니다.
Also applies to: 105-107
186-189: BannerLayout 파라미터 확장 LGTM내부 시그니처가 새 API(left/right)와 일관되도록 정리된 점 좋습니다.
217-224: leftIcon 접근성 확인선행 아이콘이 장식 목적이면 contentDescription = null 유지가 타당하지만, 의미가 있는 경우(상태 전달 등)에는 설명을 제공해야 합니다. 사용처 의도 확인 부탁드립니다.
275-277: rightIcon 표시는 onClick 전제 — 스펙 일치 여부 확인행 전체 클릭(onClick)일 때만 우측 아이콘을 보여주는 설계는 이해됩니다. onRemove가 함께 제공되면 우측 아이콘이 숨겨지는 현행 규칙이 의도와 일치하는지 확인 부탁드립니다.
425-427: 미리보기: rightIcon 사용 예시 좋습니다클릭 가능한 배너에서 기본 체브론 대체 케이스를 잘 보여줍니다.
vvvvvoin
changed the title
| replaceWith = ReplaceWith("Banner(type, description, modifier, title, leftIcon = icon, leftIconColor = iconColor, onClick, onRemove, content)") | ||
| ) |
There was a problem hiding this comment.
이건 코드래빗 말처럼 통일하는 게 좋겠는데요?
| leftIcon: Painter? = null, | ||
| leftIconColor: Color = Color.Unspecified, | ||
| rightIcon: Painter? = null, | ||
| onClick: (() -> Unit)? = null, |
There was a problem hiding this comment.
rightIconColor는 필요 없나요? 넣는 게 나아보여서요
There was a problem hiding this comment.
라이트는 버튼 스펙을 따라가서 고정으로 생각해서 색상은 따로 넣지는 않았습니당
그래도 혹시 모르니 요건 디자인팀에 확인해볼게요
| rightIcon: Painter?, | ||
| onClick: (() -> Unit)?, | ||
| onRemove: (() -> Unit)?, |
There was a problem hiding this comment.
rightIcon를 커스텀할 수 있음으로써 onClick과 onRemove의 차이에 대해 인지하기가 어려울 것으로 우려돼요.
onRemove를 지우고 onClick으로 통일하는 건 어떠신가요?
앞으로 아이콘은 onRemove나 onClick의 nullable여부로 판단해서 표시해주는 게 아닌, 그냥 커스텀 아이콘으로만 표시하도록이요.
There was a problem hiding this comment.
onClick으로 통합하면 X버튼을 눌러서 닫는 느낌이 아니라 배너 어디든 클릭해서 닫힐거 같아서 뭔가 구분이 필요해 보입니다.
요것도 디자인 의도가 좀 더 명확해야할거 같기도 하네요
| @Composable | ||
| fun Banner( | ||
| type: BannerType, | ||
| description: String, | ||
| modifier: Modifier = Modifier, | ||
| title: String? = null, | ||
| leftIcon: Painter? = null, | ||
| leftIconColor: Color = Color.Unspecified, | ||
| rightIcon: Painter? = null, | ||
| onClick: (() -> Unit)? = null, | ||
| onRemove: (() -> Unit)? = null, | ||
| content: (@Composable () -> Unit)? = null, | ||
| ) { |
There was a problem hiding this comment.
기본 파라미터 값 때문에 deprecated 된 Banner의 오버로드와 호출부의 형태가 겹쳐버리는 것 같아요. title, descripion, type만 값을 지정하면 의도와 달리 deprecated된 오버로드로 가는 것 같습니다!
기존에 라이브러리를 사용하는 곳에서 icon이 사용되지 않는 경우가 많이 없다면 이대로 두는 것도 방법일 것 같은데, 그렇지 않으면 @Deprecated 지정한 오버로드에 level = DeprecationLevel.HIDDEN을 달아두는 것도 고려해봐야 할 것 같네요. 다만 이러면 source-breaking change가 돼서 minor 버전을 올리거나 해야 할 것 같습니다
3 participants
Summary by CodeRabbit
신기능
변경/사용 중단 안내